This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3593

fyi @Atemu

This appears to change eval on many of the touched packages, what's the difference?

This appears to change eval on many of the touched packages, what's the difference?

Sorry for the late response, but I don't know.

Please investigate that, I do not feel comfortable merging this otherwise.

Alright, I found the cause.
Before, some packages were listed twice:

• In multiPkgs (because they were there by default)
• In extraPkgs (because of extraPkgs = pkgs: (appimageTools.defaultFhsEnvArgs.multiPkgs pkgs))

By removing the latter, these packages are listed only once.

Example with Zettlr (I took the pkgs from the derivations and prettified the JSON).

• After PR:
  h3py958xa6rmhv10q9xizlx5y5qlhpqf-zettlr-3.0.2-usr-target.drv.json
• Before PR:
  xjl0fnnwy3ad1czrnk6slci2kf4brp7n-zettlr-3.0.2-usr-target.drv.json

By using a text comparison tool (or a duplicate counter), you can see that "After PR" only removes duplicates.

I only tested Zettlr, but I suppose it's the same for the others.

Could you squash this into one commit? It's a tree-wide change, so that's one logical step.

Result of nixpkgs-review pr 279260 run on x86_64-linux 1

There's a conflict now. Could you also add a commit message that explains the background of the change? The contributor's manual has a section on that. It should also say something to the effect of treewide: remove multiArch=false from buildFHSEnv usages.

Other than that, this LGTM.

So, I just noticed that there are other packages that do extraPkgs = pkgs: appimageTools.defaultFhsEnvArgs.multiPkgs pkgs;, which I didn't notice before because they didn't set multiArch = false; (the only thing I searched for).
I guess I should make a separate commit for that. Or an entirely different PR.

What do you say?

35 packages that either do extraPkgs = pkgs: appimageTools.defaultFhsEnvArgs.multiPkgs pkgs; or accidentally use the top-level input.
With the latter I mean:

{ appimageTools, pandoc }:
appimageTools.wrapType2 {
  # Haa, the joys of being bamboozled by the `with;` keyword!
  # I can say that I've had experience with that, I'm definitively not still angry about the time I wasted for #248898.
  extraPkgs = pkgs: with pkgs; [ pandoc ];
}

Maybe fixing this should be a separate PR.

I guess I should make a separate commit for that. Or an entirely different PR.

Certainly a separate commit.

It fits the theme of outdated pattern/convention cleanup surrounding buildFHSEnv of this PR and is related, so I wouldn't mind having it in here.

accidentally use the top-level input.

Oh man, time for NixOS/rfcs#110...

So, there are 5 packages that use wrapAppImage instead of wrapType2.
Documentation doesn't mention the existence of the former at all.
Also, while in wrapType2 extraPkgs defaults to [ ], the same doesn't apply to wrapAppImage (which has no default).
It seems to me that wrapAppImage is only meant to be called by wrapType2 and nothing else.

Should I convert all the usages of wrapAppImage to wrapType2?
Or just put extraPkgs ? pkgs: [ ] in wrapAppImage?

I'm waiting for comments before proceeding further.

extraPkgs ? pkgs: [ ] strikes me as a sane default

Thank you!